-
Notifications
You must be signed in to change notification settings - Fork 66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add EIP7736 support #457
base: master
Are you sure you want to change the base?
Add EIP7736 support #457
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this PR. This is a good start, although it requires a lot of polishing, and probably a couple spec discussions as well. I left a few comments for you to consider, I can have another pass when you've had a look.
benchs/main.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be nice to have another benchmark that checks how much time it takes to expire the data. For example, a benchmark that inserts a few "old" leaves in the tree, then a lot more "newer" leaves, and then see how much it takes to delete all leaves.
We are not looking into bulk-expiring the tree, I think it would be insane, but to estimate what it would take to expire a few leaves per block, in case we detect that they are still present in the tree in case they are expired. Think of a "garbage collect on access" type of approach.
Here's a general explanation of how expiry works with stateless proofs: There are 4 scenarios for a given stem:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left more change requests
proof_ipa.go
Outdated
@@ -94,6 +94,7 @@ type SuffixStateDiffs []SuffixStateDiff | |||
type StemStateDiff struct { | |||
Stem [StemSize]byte `json:"stem"` | |||
SuffixDiffs SuffixStateDiffs `json:"suffixDiffs"` | |||
Resurrected bool `json:"resurrected"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to be taking more space. I would do a expiredStem
field like we have poaStems
. But no need to take action on this right now, as long as it works it's fine. This is a later-stage optimization.
546eedd
to
8a00f80
Compare
revert unnecessary changes remove unnecessary changes
8a00f80
to
e558f36
Compare
…ip7736/master # Conflicts: # encoding.go # tree.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This PR must be updated to refresh the period upon a read
- Also, I don't think that the tests are actually checking that an expiration has been proven. but maybe I misundertsood.
- It also seems to me that inserting a sibling to an expired node isn't tested.
epoch.go
Outdated
"encoding/binary" | ||
) | ||
|
||
type StatePeriod uint16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I already asked that, but why uint16
? On some archs with strong alignment constaints, that could cause a lot of slowdowns, and, however small that slowdown is, I don't know why we would pay that cost?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that if this is because we don't want to take too much space with the leaf encoding, we can always store it as a uint16
(or even a uint8
, that's 128 years before we need to upgrade!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per our convo, this is a premature optimization and this comment should be discarded. We can look into it in the future.
This reverts commit b670596.
93b1fb4
to
2950a74
Compare
Change Log (5/1/2025):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's getting into shape! I'm not sure that i understand the point of skipUpdate
: why shouldn't the counter/commitment be updated upon revival? We can think of a use case where someone resurects an account for someone else, and doesn't directly write/read from that account.
return errMissingNodeInStateless | ||
case Empty: | ||
// TODO(weiihann): double confirm this | ||
return errors.New("cannot revive an empty node") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you go through a tree and the node that you are given indicates that the commitment is non-zero, you should definitely error. But if the commitment is 0, there's a case to be made that the tx should not fail... although, if it's simpler to handle, then let's just error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll just leave it as error for now and add the scenario you mentioned in the comments
It's because of stateless proof. If the pre-state tree contains expired nodes, it has to be revived first. If the counter is updated directly, the commitment of the node and the tree changes. Pre-state tree has to be revived such that it preserves the original values and counter, so that it can generate the proof items properly. Though there might be better ways of doing this. Another alternative is to cache the previous counter and previous commitment's information, but that makes the logic more complex. I would have to look at how the stateless proof is generated in geth and revisit this if needed. |
Primary changes:
StateEpoch
typeGet
,Insert
andDelete
to includeStateEpoch
. If trying to access expired leaf node, returns error.lastEpoch
field inLeafNode
, included in serialization and node commitment calculationExpiryLeafNode
struct